-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat: Add support for user nameGenerator function
#4773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@nlemoine I guess the TLDR is I'm not a fan of the approach, as it is too complex for your use case. I am open to you arguing your case for this approach over a simple flag to update the |
|
Hey @heath-freenome, thanks for the thorough review! I really appreciate your time and feedback. I understand your comments and will try to address your concerns, particularly about using Backend need examples
The
|
| const itemIdSchema = schemaUtils.toIdSchema(itemSchema, itemIdPrefix, itemCast, idPrefix, idSeparator); | |
| // Compute the item UI schema using the helper method | |
| const itemUiSchema = this.computeItemUiSchema(uiSchema, item, index, formContext); | |
| return this.renderArrayFieldItem({ | |
| key, | |
| index, | |
| name: name && `${name}-${index}`, |
I gave a quick and dirty try to that approach and it gave me:
<input id="root[listOfStrings]_0" name="root[listOfStrings]_0" class="form-control" label="listOfStrings-0" required="" placeholder="" type="text" aria-describedby="root[listOfStrings]_0__error root[listOfStrings]_0__description root[listOfStrings]_0__help" value="foo">But most importantly, it would bring some more major issues, IMHO.
HTML Requires different IDs and names
Even with a toIdSchema working solution (e.g. generating bracketted ids), this isn't just about backend preferences - HTML itself requires different values for id and name attributes in several fundamental cases:
1. Radio button groups
Radio buttons in the same group must share the same name but have different id values:
<!-- ✅ Same name, different IDs -->
<input type="radio" id="root_color_0" name="root[color]" value="red">
<label for="root_color_0">Red</label>
<input type="radio" id="root_color_1" name="root[color]" value="blue">
<label for="root_color_1">Blue</label>
<!-- ❌ Using ID as name breaks radio grouping -->
<input type="radio" id="root_color_0" name="root_color_0" value="red">
<input type="radio" id="root_color_1" name="root_color_1" value="blue">Radio group is already ok in current RJSF state but if id is used as name, this will no longer be the case.
2. Checkbox arrays
Multiple checkboxes that submit as an array need the same base name:
<!-- ✅ PHP-style array submission -->
<input type="checkbox" id="root_hobbies_0" name="root[hobbies][]" value="reading">
<input type="checkbox" id="root_hobbies_1" name="root[hobbies][]" value="gaming">
<input type="checkbox" id="root_hobbies_2" name="root[hobbies][]" value="cooking">
<!-- Submits as: hobbies = ['reading', 'gaming'] when multiple are checked -->
<!-- ❌ Using ID as name -->
<input type="checkbox" id="root_hobbies_0" name="root_hobbies_0" value="reading">
<input type="checkbox" id="root_hobbies_1" name="root_hobbies_1" value="gaming">
<!-- Submits as separate unrelated values, not an array! -->Same as above.
3. Select multiple
Similar issue with multi-select dropdowns:
<!-- Needs array notation in name -->
<select id="root_skills" name="root[skills][]" multiple>
<option value="js">JavaScript</option>
<option value="python">Python</option>
</select>Why brackets in IDs are complicated
Even if we wanted to use the same bracketed format for both ID and name (like root[tasks][0]), it would break or weaken major parts of the web stack:
CSS selectors
/* Brackets are special characters in CSS */
#root[tasks][0] { } /* ❌ BREAKS - interpreted as attribute selector */
/* Would require escaping everywhere */
#root\[tasks\]\[0\] { } /* Ugly and error-prone */
/* Current RJSF approach */
#root_tasks_0 { } /* ✅ Clean, simple, works everywhere */JavaScript DOM Queries
// querySelector fails with special characters
document.querySelector('#root[tasks][0]') // ❌ SyntaxError
// Would need escaping
document.querySelector('#root\\[tasks\\]\\[0\\]') // Complex escaping
// Current clean approach
document.querySelector('#root_tasks_0') // ✅ Works everywhereI would have loved a much simpler solution too. Adding a flag in toIdSchema feels like a workaround more than a robust and flexible alternative.
I tried to think of a one that brings the minimal possible changes. It can surely be improved (JS/React is not my strong suit) and I will address your review comments if you change your mind.
However, if you are still not convinced and don't see any benefits in bringing separate logic for id and name attributes, I would totally understand. I'm ready to work on a toIdSchema alternative, since I'd really like that feature to make it for version 6.
What do you think? I'll be happy to refactor the implementation based on your preferences.
|
@nlemoine Thank you for the well documented explanation. It has convinced me and my compatriot @nickgros that there is merit in your approach. So much so, that I've just pushed a PR that does a major breaking refactor of the system to simplify your work quite a bit. In order to implement your feature, I am going to recommend the following approach after my PR merges:
|
|
HI @heath-freenome, Thank you so much for paying so much attention to that issue and bringing so much changes to that purpose! 🙌 I'll try to update the PR as soon as I can and will keep posted. |
c921892 to
621a1fa
Compare
|
Quick questions @heath-freenome
|
Right now the
I'm fine with NOT providing them. You will still need something to test with so perhaps adding them is helpful? |
Actually, no, multiple checkboxes are considered an array. So they need brackets ( The name is only generated in |
I'm not sure I understand why the indexes are needed for the name of checkboxes in a checkbox group? Since submitting the form will automatically cause the html to return |
My suggestion would be to create a new utility function in |
|
@nlemoine Will you have this feature completed sometime in the next 10 days? I'd like to get it into a prerelease of v6 before we go live with the official release |
85e8c6f to
69fe928
Compare
|
Hi @heath-freenome, Sorry for the delay - I had a busy week with limited time to work on this. I've finally managed to put everything together. I couldn't go with the solution you initially suggested. The key insight is that for multiple This is why I added an optional
Can you review this approach before I propagate these changes to all the theme libraries? I'd like your approval first before going further. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nlemoine I like that approach as well. Go ahead and do the bigger update. Don't forget the docs and the CHANGELOG.md as well
6665a60 to
78f0294
Compare
Co-authored-by: Heath C <51679588+heath-freenome@users.noreply.github.com>
e501a38 to
a67c3e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few suggested changes. I also have a PR to convert the ArrayField class component into a set of stateless functional components. If mine merges first, you will likely have to rework your ArrayField changes in that file. If you can get the changes made in the next few hours, then I will merge yours and update mine instead
| const displayLabel = schemaUtils.getDisplayLabel(schema, uiSchema, globalUiOptions); | ||
|
|
||
| // For custom widgets with multiple=true, generate a fieldPathId with isMultiValue flag | ||
| const multiValueFieldPathId = toFieldPathId('', globalFormOptions, fieldPathId, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To prevent rerender due to new fieldPathId that is exactly the same (but different storage), can you import the new useDeepCompareMemo function from @rjsf/utils and do this?
| const multiValueFieldPathId = toFieldPathId('', globalFormOptions, fieldPathId, true); | |
| const multiValueFieldPathId = useDeepCompareMemo(toFieldPathId('', globalFormOptions, fieldPathId, true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what I should do about this. I can't use that React hook here in the current state of my PR.
React Hook "useDeepCompareMemo" cannot be called in a class component. React Hooks must be called in a React function component or a custom React Hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll merge yours then update it in mine
| const displayLabel = schemaUtils.getDisplayLabel(schema, uiSchema, globalUiOptions); | ||
|
|
||
| // For multi-select widgets, generate a fieldPathId with isMultiValue flag | ||
| const multiValueFieldPathId = toFieldPathId('', globalFormOptions, fieldPathId, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To prevent rerender due to new fieldPathId that is exactly the same (but different storage), can you import the new useDeepCompareMemo function from @rjsf/utils and do this?
| const multiValueFieldPathId = toFieldPathId('', globalFormOptions, fieldPathId, true); | |
| const multiValueFieldPathId = useDeepCompareMemo(toFieldPathId('', globalFormOptions, fieldPathId, true)); |
| const displayLabel = schemaUtils.getDisplayLabel(schema, uiSchema, globalUiOptions); | ||
|
|
||
| // For file widgets with multiple=true, generate a fieldPathId with isMultiValue flag | ||
| const multiValueFieldPathId = toFieldPathId('', globalFormOptions, fieldPathId, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To prevent rerender due to new fieldPathId that is exactly the same (but different storage), can you import the new useDeepCompareMemo function from @rjsf/utils and do this?
| const multiValueFieldPathId = toFieldPathId('', globalFormOptions, fieldPathId, true); | |
| const multiValueFieldPathId = useDeepCompareMemo(toFieldPathId('', globalFormOptions, fieldPathId, true)); |
Authored-by: Heath C <51679588+heath-freenome@users.noreply.github.com>
1ff0b25 to
a318c15
Compare
|
I made some of the changes you suggested but didn't include the This is unrelated but may I ask if there's a recommended folder structure for themes? I noticed some of them follow this structure: While others are structured like: Any advice for building a custom theme? |
I'd go with the second pattern... At some point I'll get around to making all the themes have the same directory structure. |
|
@nlemoine Even though I merged this, I just realized that you don't actually have any tests that verify adding a name generator does what you expect. Can you, at minimum, add some snapshot tests that show off the name generators in action? You would simply have to update the |
|
Hi @heath-freenome, Thanks for merging this! Sorry, I indeed forgot to add some tests. You will extensive tests in #4809 |


Reasons for making this change
Fixes #4693
If this is related to existing tickets, include links to them as well. Use the syntax
fixes #[issue number](ex:fixes #123).Checklist
npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:updateto update snapshots, if needed.